Add bwc for parsing mappings from dynamic templates #67763
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During refactoring of the mapper parsing code, the part that checks for unknown mapper parameters and throws an error when one is found was moved to FieldMapper.Builder#parse which gets also executed when applying matching dynamic templates. However, dynamic templates introduced during the 7.x versions may still contain arbitrary parameters, although we have deprecation warnings for that in place on latest 7.x now. When using these templates, indexing a new document with a new field that triggers one of these mappings to be parsed can create an error as demonstrated in elastic#66765. Instead we need to be lenient in these cases and simply ignore the unknown parameter while issuing a deprecation warning here as well. Closes elastic#66765
cbuescher
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
romseygeek
reviewed
Jan 20, 2021
| .put("index.version.created", VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0)) | ||
| ).setMapping(mapping).get()); | ||
| client().prepareIndex(indexName).setSource("{\"foo\" : \"bar\"}", XContentType.JSON).get(); | ||
| // assertNoFailures((client().admin().indices().prepareRefresh(indexName)).get()); |
There was a problem hiding this comment.
Should this be commented out?
cbuescher
commented
Jan 20, 2021
| @@ -990,8 +990,21 @@ public final void parse(String name, ParserContext parserContext, Map<String, Ob | |||
| iterator.remove(); | |||
| continue; | |||
| } | |||
| throw new MapperParsingException("unknown parameter [" + propName | |||
| + "] on mapper [" + name + "] of type [" + type + "]"); | |||
| if (parserContext.isFromDynamicTemplate() && parserContext.indexVersionCreated().before(Version.V_8_0_0)) { | |||
There was a problem hiding this comment.
I don't think we can have a dynamic template in an index >8.0.0 with a malformed parameter since we cannot add it, but I put the version check here regardless for safety. @romseygeek wdyt?
romseygeek
reviewed
Jan 20, 2021
| // ); | ||
| // assertThat(ex.getMessage(), containsString("dynamic template [my_template] has invalid content")); | ||
| assertNoFailures((client().admin().indices().prepareRefresh(indexName)).get()); | ||
| assertHitCount(client().prepareSearch(indexName).get(), 1); |
There was a problem hiding this comment.
Can we assert that we get a deprecation warning from the index action?
There was a problem hiding this comment.
I was looking at how to do that in Integ tests but couldn't find any pointers. ESIntegTestCase#enableWarningsCheck makes me think its not possible here, but if you have pointers to tests doing this I'm happy to add it here.
There was a problem hiding this comment.
You can call getWarnings() on the response
There was a problem hiding this comment.
Unfortunately this IndexResponse is no org.elasticsearch.client.Response, will check if there are other ways...
There was a problem hiding this comment.
If there's no way of doing it then let's get this merged anyway.
cbuescher
pushed a commit
to cbuescher/elasticsearch
that referenced
this pull request
Jan 26, 2021
During refactoring of the mapper parsing code, the part that checks for unknown mapper parameters and throws an error when one is found was moved to FieldMapper.Builder#parse which gets also executed when applying matching dynamic templates. However, dynamic templates introduced during the 7.x versions may still contain arbitrary parameters, although we have deprecation warnings for that in place on latest 7.x now. When using these templates, indexing a new document with a new field that triggers one of these mappings to be parsed can create an error as demonstrated in elastic#66765. Instead we need to be lenient in these cases and simply ignore the unknown parameter while issuing a deprecation warning here as well. Closes elastic#66765
cbuescher
pushed a commit
to cbuescher/elasticsearch
that referenced
this pull request
Jan 26, 2021
During refactoring of the mapper parsing code, the part that checks for unknown mapper parameters and throws an error when one is found was moved to FieldMapper.Builder#parse which gets also executed when applying matching dynamic templates. However, dynamic templates introduced during the 7.x versions may still contain arbitrary parameters, although we have deprecation warnings for that in place on latest 7.x now. When using these templates, indexing a new document with a new field that triggers one of these mappings to be parsed can create an error as demonstrated in elastic#66765. Instead we need to be lenient in these cases and simply ignore the unknown parameter while issuing a deprecation warning here as well. Closes elastic#66765
cbuescher
pushed a commit
that referenced
this pull request
Jan 26, 2021
During refactoring of the mapper parsing code, the part that checks for unknown mapper parameters and throws an error when one is found was moved to FieldMapper.Builder#parse which gets also executed when applying matching dynamic templates. However, dynamic templates introduced during the 7.x versions may still contain arbitrary parameters, although we have deprecation warnings for that in place on latest 7.x now. When using these templates, indexing a new document with a new field that triggers one of these mappings to be parsed can create an error as demonstrated in #66765. Instead we need to be lenient in these cases and simply ignore the unknown parameter while issuing a deprecation warning here as well. Closes #66765
cbuescher
pushed a commit
that referenced
this pull request
Jan 26, 2021
During refactoring of the mapper parsing code, the part that checks for unknown mapper parameters and throws an error when one is found was moved to FieldMapper.Builder#parse which gets also executed when applying matching dynamic templates. However, dynamic templates introduced during the 7.x versions may still contain arbitrary parameters, although we have deprecation warnings for that in place on latest 7.x now. When using these templates, indexing a new document with a new field that triggers one of these mappings to be parsed can create an error as demonstrated in #66765. Instead we need to be lenient in these cases and simply ignore the unknown parameter while issuing a deprecation warning here as well. Closes #66765
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During refactoring of the mapper parsing code, the part that checks for unknown
mapper parameters and throws an error when one is found was moved to
FieldMapper.Builder#parse which gets also executed when applying matching
dynamic templates. However, dynamic templates introduced during the 7.x versions
may still contain arbitrary parameters, although we have deprecation warnings
for that in place on latest 7.x now. When using these templates, indexing a new
document with a new field that triggers one of these mappings to be parsed can
create an error as demonstrated in #66765. Instead we need to be lenient in
these cases and simply ignore the unknown parameter while issuing a deprecation
warning here as well.
Closes #66765